Add Subtitles row to MovieDetails component. Fix incorrect initially selected subtitle stream.#728
Conversation
Encapsulates the selected video, audio, subtitle streams into a MovieDetailsStreamSummary component. Adds a Subtitles row to the MovieDetails component to indicate to the user which subtitle stream is selected, along with the video and audio streams. Fixes the initial selected subtitles stream in MovieOptions. The initial selected subtitles stream comes to us from the Jellyfin server and if it is not present there, then we default to None.
|
I understand this is a rather large charge consisting of multiple things, but I was on a roll and wanted to get some feedback on all of it. The big pieces:
Let me know if you'd prefer that I break this up into a series of smaller pull requests, or if there are any of those pieces that we simply do not want. |
|
Very cool! I love the display component. In my functionality testing, found an unhappy path. I have a video that has both embedded subtitles and an external subtitle file. If I select the embedded subtitles prior to playback, it does change the displayed subtitle track, but on playback, the external track is played and shows as selected in the subtitle popup. The external track is the default according to the server, but I manually selected the embedded track. So my selection should override the default selection. selection.mp4 |
components/movies/MovieDetails.bs
Outdated
| mediaStreams = itemData.mediaStreams | ||
|
|
||
| for i = 0 to mediaStreams.Count() - 1 | ||
| if mediaStreams[i].Type = "Audio" |
There was a problem hiding this comment.
Add AUDIO to the MediaStreamType ENUM and use it here.
if isStringEqual(chainLookup(mediaStreams[i], "Type"), MediaStreamType.AUDIO)
components/movies/MovieDetails.bs
Outdated
| selectedSubtitleStreamTitle = "None" | ||
| for i = 0 to itemData.mediaStreams.Count() - 1 | ||
| stream = itemData.mediaStreams[i] | ||
| if stream.Type = "Subtitle" |
There was a problem hiding this comment.
Use enum, isStringEqual, and chain lookup.
components/movies/MovieDetails.bs
Outdated
| function findStreamSummaryNode() as dynamic | ||
| node = m.top.findNode("streamSummary") | ||
| return node | ||
| end function | ||
|
|
||
| sub setRotationWarning(rotationWarning as string) | ||
| findStreamSummaryNode().rotationWarning = rotationWarning | ||
| end sub | ||
|
|
||
| sub setSelectedVideoStreamTitle(title as string) | ||
| findStreamSummaryNode().selectedVideoStreamTitle = title | ||
| end sub | ||
|
|
||
| sub setVideoStreamCount(count as integer) | ||
| findStreamSummaryNode().videoStreamCount = count | ||
| end sub | ||
|
|
||
| sub setSelectedAudioStreamTitle(title as string) | ||
| findStreamSummaryNode().selectedAudioStreamTitle = title | ||
| end sub | ||
|
|
||
| sub setAudioStreamCount(count as integer) | ||
| findStreamSummaryNode().audioStreamCount = count | ||
| end sub | ||
|
|
||
| sub setSelectedSubtitleStreamTitle(title as string) | ||
| findStreamSummaryNode().selectedSubtitleStreamTitle = title | ||
| end sub | ||
|
|
||
| sub setSubtitleStreamCount(count as integer) | ||
| findStreamSummaryNode().subtitleStreamCount = count |
There was a problem hiding this comment.
All of these are assuming findStreamSummaryNode() returns a valid node. Defensive coding requires us to confirm it is valid prior to using it, otherwise it'll crash out.
Additionally, you're finding the node over and over again inside of code that will most likely be run multiple times. I'm cool running findnode on demand for things that wait for an event, like a keypress, but since we will use this in most scenarios, put the find in the init as an m. variable and just use it in all these functions.
There was a problem hiding this comment.
It looks like there isn't any checking for isValid() when we use findNode() in the init() subroutine. Is it guaranteed we'll find it while in the scope of init()?
When I assign the streamSummary node to m.streamSummary, does that remove the need to check for validity in all of these setters?
There was a problem hiding this comment.
Nope, you'll still need to check it's valid before using it, it simply saves the time findnode() would take to traverse the XML to find the node as the node reference is saved/remembered if it's found.
| @@ -0,0 +1,12 @@ | |||
| sub onChangeTitle() | |||
| streamTitle = m.top.title | |||
| node = m.top.findNode("title") | |||
There was a problem hiding this comment.
Assumes the node is found. Before accessing the text param, we must perform an isValid() check.
| count = m.top.count | ||
| node = m.top.findNode("count") | ||
| additionalCount = count - 1 | ||
| node.text = additionalCount > 0 ? "+" + stri(additionalCount).trim() : "" |
There was a problem hiding this comment.
Assumes the node is found. Before accessing the text param, we must perform an isValid() check.
|
@1hitsong thank you for all of your fantastic feedback and suggestions! |
Thank you so much for exercising this edge case! I don't have any media files with external subtitle tracks. If I have difficulty setting up the same scenario, I'll reach out for help in reproducing this on my end. |
|
You're very welcome. Just so you know some of the WHY we use the helper functions.
|
Thank you for all of these explanations! |
Co-authored-by: 1hitsong <3330318+1hitsong@users.noreply.github.com>
Should I just use Keeping track of what is case insensitive and what isn't is wild! 😆 😢 |
If you're using the same value over and over, use chainlookup once and save the value to a variable. Or, if you're using an object in the chain over and over, save it to a variable then use it as the root element in chainLookup(). The reason is so you're not looping through the same properties checking for valid over and over and over again. Once we know it's valid, it's just wasted cycles checking the same thing repeatedly.
🤘🏻 Exactly! |
- utilize isValid, isValidAndNotEmpty, isStringEqual, chainLookup, etc. - add MediaStreamType enum values - find XML nodes in `init()` subroutines
|
I think I addressed all of the pull request comments. I still need to investigate the external subtitles bug. |
Any luck tracking it down? |
Sorry, life got busy! I expect I'll sadly be busy this upcoming weekend, too. I did add external subtitles to a show on my system. I wasn't able to get them to render during playback (not sure why). But I don't expect rendering them is a concern for this PR. I did notice in your video that it looked like the Subtitle selection quickly switches back to external in between dismissing the MovieOptions and starting playback. If I recall, that was the only lead I had to work with when I was last able to sit down and focus on this. |
2 participants
Changes
Encapsulates the selected video, audio, and subtitle streams into a MovieDetailsStreamSummary component. Fix incorrect initially selected subtitle stream in MovieOptions.
Adds a Subtitles row to the MovieDetails component to indicate to the user which subtitle stream is selected, along with the video and audio streams.
Fixes the initial selected subtitles stream in MovieOptions. The initial selected subtitles stream comes to us from the Jellyfin server and if it is not present there, then we default to None.
Issues
Fixes #526 and #626